[74542] Add an authentication strategy for the wikis module#23115
Conversation
- Add `BearerToken` auth strategy and all the code required by it - Updated `UserQuery` to use `BearerToken` auth strategy instead of just passing the token
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
I'd suggest to extend the scope of this PR to all the queries (and callers of those queries) that we already built. Mostly, because this gives a more complete picture on the real requirements we have.
We already have services calling queries. And we already have working queries for the internal provider. In its current state, this PR wouldn't be able to work for internal provider queries, because there is no internal.authentication.user_bound strategy registered.
Trying to apply the current state of the PR to the existing services would've also shown that these now need to obtain token information that's:
- hard to obtain for a service (they would have to go to the access tokens table as well)
- not necessary for all queries
One additional note for internal.authentication.user_bound: Assuming we build authentication strategies to accept a user: So far the internal queries implicitly use User.current for authentication. I think once auth strategies are in place in makes sense to pass the user into the queries via the auth strategy. E.g. a WikiPage.visible could then become something like
auth_strategy.call do |user|
WikiPage.visible(user)
endto clean up the code
For internal queries
…ub.com/opf/openproject into implementation/74542-add-auth-strategy
Alright, let's do it. I've added some code and pushed it as well, but it still WIP. |
| def call | ||
| if @model.new_record? || @force_update | ||
| origin_result = @integration.extract_origin_user_id(@token) | ||
| origin_result = @integration.extract_origin_user_id(@token.user) |
There was a problem hiding this comment.
🟢 I assume this will work as well, but given that the RemoteIdentity is created based on @user, I'd have expected that we pass that here as well
| origin_result = @integration.extract_origin_user_id(@token.user) | |
| origin_result = @integration.extract_origin_user_id(@user) |
There was a problem hiding this comment.
I accepted this code at the beginning, but then specs showed me that it's wrong: the storages expecting token here, not a user. So I rolled back to just @token and then @token.user for wikis for extract_origin_user_id. We could refactor the code to accept user, but I'd say it's out of this PR's topic. I'll merge the PR for now, but this comment is a reminder for the improvement 😄
There was a problem hiding this comment.
Oh, I see. Both storages and wikis must implement the same interface here.
Understood, so be it.
I'll merge the PR for now, but this comment is a reminder for the improvement 😄
It took me two minutes to find this comment again after seeing it in an email, because it was part of a resolved discussion ;-P
There was a problem hiding this comment.
Oh crap, I expanded the code with
but the conversation was resolved because I accepted the code suggestion 🤦🏼
Co-authored-by: Jan Sandbrink <j.sandbrink@openproject.com>
Update the method signature
…ub.com/opf/openproject into implementation/74542-add-auth-strategy
| end | ||
|
|
||
| def call(http_options: {}, **) | ||
| token = fetch_user_token.value_or { return Failure(it) } |
There was a problem hiding this comment.
🟢 I might be wrong, but I think this could now by monadified:
fetch_user_token.bind do |token|
yield OpenProject.httpx.bearer_auth(token.access_token).with(http_options)
endThis should effectively be the same:
- on success the block is called and we authenticate with the token, returning what the yield returns
- on failure, we short-circuit and return the failure
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
Assuming that specs are happy, this is now shaped the way that I'd have expected it.
From here we should be able to scale authentication to all queries 🤞
Because storages expecs token here not user
2 participants
Ticket
Follow up for the 74542
What are you trying to accomplish?
BearerTokenauth strategy and all the code required by itUserQueryto useBearerTokenauth strategy instead of just passing the tokenMerge checklist